[EXPERIMENT] New models - #216

@haikoschol re

Where did this information come from?

https://docs.djangoproject.com/en/3.1/releases/3.1/#jsonfield-for-all-supported-database-backends , django/django#12392 , it was a GSoC 2019 project :)

@sbs2001 re:

Problem 1 : A row in VulnerabilityReference does not properly encapsulate the reference ids and urls. Consider an importer encounters 2 distinct reference ids for a vulnerability, say USN-A and DSA-B and 2 corresponding urls 'www.USN-A.org' and 'www.DSA-B.org'. With the current implementation, A VulnerabilityReference can contain only single url and a single reference_id. What ends up happening is we either end up making 4 VulnerabilityReference row, like
|....|USN A | NULL |
|....|DSA B | NULL |
|....|NULL | 'www.USN-A.org' |
|....|NULL | 'www.DSA-B.org'|
or have rows of combinations of reference_id and url , which might be unrelated.

Rather I think we should get in this example only two rows as:

@pombredanne re

Rather I think we should get in this example only two rows as:

Exactly, that's the expected thing. FWIW there are cases where there are reference ids without corresponding urls and vice versa but that's not the issue.

The problem is, as the current Importer implementation is there is no way for us to 'tell' the importer mechanism which reference id is related to which url. I really don't have any good ideas to fix that than manually specifying which id has which url, but then it gets really messy in the corner cases which are mentioned above(not really corner cases) .

Check https://github.com/nexB/vulnerablecode/blob/58d0376e7319d06387662cb393f3c39d9893088d/vulnerabilities/import_runner.py#L111 .

So the next best thing I could think of is throw all the urls and ref_ids in a JSONField.

@pombredanne @haikoschol btw please mention things which you think are correctly implemented, once I have your aproval I can get started on that thing ASAP.

re: https://github.com/nexB/vulnerablecode/blob/58d0376e7319d06387662cb393f3c39d9893088d/vulnerabilities/import_runner.py#L111
I think this code is the faulty one, you should not invoke twice get_or_create() and the model

            for id_ in advisory.reference_ids:
                models.VulnerabilityReference.objects.get_or_create(
                    vulnerability=vuln, reference_id=id_)

            for url in advisory.reference_urls:
                models.VulnerabilityReference.objects.get_or_create(vulnerability=vuln, url=url)

The issue here is NOT the DB model but the Advisory data structure in https://github.com/nexB/vulnerablecode/blob/58d0376e7319d06387662cb393f3c39d9893088d/vulnerabilities/data_source.py#L60

This

    reference_urls: Sequence[str] = dataclasses.field(default_factory=list)
    reference_ids: Sequence[str] = dataclasses.field(default_factory=list)

should be instead its own data structure that strictly mirrors the DB model, e.g. more or less something like that:
https://github.com/nexB/vulnerablecode/blob/58d0376e7319d06387662cb393f3c39d9893088d/vulnerabilities/models.py#L53
And may be the Django models can be used as-is too, without "saving" them? research needed?

@dataclasses.dataclass
class VulnerabilityReference: # use same name as model for now
    source = str
    reference_id = str
    url = str

[.....]

@dataclasses.dataclass
class Advisory
    references: Sequence[VulnerabilityReference] = dataclasses.field(default_factory=list)

Then the code in Importers at https://github.com/nexB/vulnerablecode/blob/58d0376e7319d06387662cb393f3c39d9893088d/vulnerabilities/import_runner.py#L111 would be more or less something like this:

# build indexes of existing references to either update or create new ones
# and avoid creating duplicates
refs_by_id_url = {(ref.id, ref.url): ref for ref in vuln.references if ref.id and ref.url}
refs_by_id = {ref.id: ref for ref in vuln.references if ref.id}
refs_by_url = {ref.url: ref for ref in vuln.references if ref.url}

for adv_ref in advisory.references:
    if adv_ref.reference_id and adv_ref.url:
        existing_ref = refs_by_id_url.get((adv_ref.id, adv_ref.url))
    elif adv_ref.reference_id:
        existing_ref = refs_by_id.get(adv_ref.id)
    elif adv_ref.reference_url:
        existing_ref = refs_by_id.get(adv_ref.url)
            
    if existing_ref:
        # update ref as neeed
        if adv_ref.summary and not existing_ref.summary:
            existing_ref.summary = adv_ref.summary
        if adv_ref.reference_id and not existing_ref.reference_id:
            existing_ref.reference_id = adv_ref.reference_id
        if adv_ref.url and not existing_ref.url:
            existing_ref.url = adv_ref.url
        if adv_ref.source and not existing_ref.source:
            existing_ref.source = adv_ref.source
        existing_ref.save()
    else:
        new_ref = VulnerabilityReference(
            vulnerability=vuln, 
            reference_id=ref.reference_id, 
            url=adv_ref.url,
            summary=adv_ref.summary,
            source=adv_ref.source,
        )
        new_ref.save()

@pombredanne

And may be the Django models can be used as-is too, without "saving" them? research needed?

Yes they can be used, with one caveat , as they don't a pk(yet) they are unhashable, hence can't be put in sets. I have experienced problems due to this in #219 , where bulk inserting/updating was done. But simple workaround for this exists.

I think this code is the faulty one, you should not invoke twice get_or_create() and the model

It is, @haikoschol and I had chat about this awhile ago on our gitter channel. The code was written assuming there is no relation between url and the id , which is true in some cases, while false in others, hence the assumption.

This can be actually solved by tweaking the advisory model albeit.

............
class Advisory : 
............
   vulnerability_references = Sequence[Mapping[str,str]]

eg value of vulnerability_references = [{'url':'usn123.com','reference_id':'usn-123'}]

In the importer

for vulnerability_reference in advisory.vulnerability_references : 
    VulnerabilityReference.objects.get_or_create(**vulnerability_reference)

IMHO the get_or_create is too simplistic and in most case an actual object with named attributes is cleaner than a mapping, hence why I do not like too much Sequence[Mapping[str,str]]

Also with:

for vulnerability_reference in advisory.vulnerability_references : 
    VulnerabilityReference.objects.get_or_create(**vulnerability_reference)

... you are missing out that thing should be merged and updated rather than just created or got IMHO

The code was written assuming there is no relation between url and the id , which is true in some cases, while false in others, hence the assumption.

IMHO there is always a relation between the two.

@pombredanne re

... you are missing out that thing should be merged and updated rather than just created or got IMHO

ack :) I messed up , but that's minor we can handle that, by doing something along the lines of the snippet you posted above.

in most case an actual object with named attributes is cleaner than a mapping, hence why I do not like too much Sequence[Mapping[str,str]]

That's a valid point in this case, how about

class VulnerabilityReferenceArgs : 
 url : str = ''
 reference_id : str =''
 def __post_init__(self) : 
  if not any([self.url, self.reference_id]):
      raise SomeError

And have Sequence[VulnerabilityReferenceArgs] instead

@sbs2001

That's a valid point in this case, how about

class VulnerabilityReferenceArgs : 
 url : str = ''
 reference_id : str =''
 def __post_init__(self) : 
  if not any([self.url, self.reference_id]):
      raise SomeError

And have Sequence[VulnerabilityReferenceArgs] instead

👍
that works (and we can find a better name than "VulnerabilityReferenceArgs" :))

Repaste from https://gitter.im/aboutcode-org/vulnerablecode

Shivam Sandbhor
@sbs2001
Jul 04 16:21
@pombredanne , I think we finally have some consensus on VulnerabilityReference at #216 , the conclusion being use FK of Importer as a source and don't touch anything else. The root of problem is in indeed in Advisory right ?

Which meant we would have multiple VulnerabilityReference for a given pair of Vulnerability and Source.

Now the question was of how to relate a VulnerabilityScore to a Vulnerability. Previously we thought this would best fit, as shown in the top comment, i.e with a FK of vulnerabilityreference because the assumption was we would have 1 VulnerabilityRef per pair of Source and Vulnerability. The assumption is not true anymore.

Shivam Sandbhor
@sbs2001
Jul 04 17:50
@pombredanne do you think VulnerabilityScore is still positioned right, especially since there can't be unique_together=('vulnerability', 'source') constraint on VulnerabilityReference ?
Philippe Ombredanne
@pombredanne
Jul 04 19:14
@sbs2001 IMHO a score is for a reference, is it?

Philippe Ombredanne
@pombredanne
Jul 04 19:51
in all cases, one or more scores would be for one VulnerabilityReference

Shivam Sandbhor
@sbs2001
I wanted to highlight the fact that
There would be creation of VulnerabilityReference for each distinct url I found referencing to CVE-2016-0778 at https://nvd.nist.gov/vuln/detail/CVE-2016-0778.
More like
vref1 = VulnerabilityReference(vulnerability=v1, ref_id='', url= https://nvd.nist.gov/vuln/detail/CVE-2016-0778, source=NVD)
vref2 = VulnerabilityReference(vulnerability=v1, ref_id='', url= http://kb.juniper.net/InfoCenter/index?page=content&id=JSA10734, source=NVD)
Now in my scores I have to create :
vscore1=VulnerabilityScore(vuln_ref=vref1, score=8.1, type=cvss3)
vscore2=VulnerabilityScore(vuln_ref=vref1, score=4.6, type=cvss2)
vscore1=VulnerabilityScore(vuln_ref=vref2, score=8.1, type=cvss3)
vscore2=VulnerabilityScore(vuln_ref=vref2, score=4.6, type=cvss2)

VulnerabilityScore should be related to a VulnerabilityReference and not to a Vulnerability

@sbs2001 in reply to #216 (comment)

Issue 1 :
You can do this, which doesn't make any sense.
In [1]: from vulnerabilities import models
In [4]: vp1 = models.ImpactedPackage.objects.create(vulnerability=v1, package=p1)
In [5]: vp2 = models.ResolvedPackage.objects.create(vulnerability=v1, package=p1)
This is pure garbage, nothing can be interpreted from these entries.

This is only a theoretical problem. Just do not do it :)
And if you want to guard from it, ensure in code that this fails if you attempt this with proper overrides in the model/maneger.

Issue 2 : Check https://github.com/nexB/vulnerablecode/blob/58d0376e7319d06387662cb393f3c39d9893088d/vulnerabilities/import_runner.py#L121 , I am not sure I understand the exact issue but it's something along the lines that updating vulnerability status of a already existing package is not possible. @haikoschol can you explain this, with a snippet?

Having a single table, changes delete to an update(of the flag), which bypasses this issue.

Note sure I get the details, but I think we have abused using get_or_create when things are often more complex than that and we want to do many more checks that just a simple get to ensure we have good data.

VulnerabilityScore should be related to a VulnerabilityReference and not to a Vulnerability

I thought it would be ok since the old models had score in Vulnerability NOT in VulnerabilityReference.

What we are trying to acheive here is to store multiple scores for a single Vulnerability.

Using score related VulnerabilityReference makes it very tricky to figure out of which VulnerabilityReference , is the VulnerabilityScore based of.

What we are trying to acheive here is to store multiple scores for a single Vulnerability.

IMHO rather we are trying to attach the score to a given reference which is really the only thing we can be sure of. And a ref (with its score) may be linked in multiple Vulnerabilities

Using score related VulnerabilityReference makes it very tricky to figure out of which VulnerabilityReference , is the VulnerabilityScore based of.

Why so?

I see it this way, for a given vulnerability

• ref1(NVD CVE) says the score type cvss2 is 5.0
• ref1(NVD CVE) says the score type cvss3 is 6.0
• ref2(RedHat CVE) says the score type cvss2 is 7.0
  which becomes really straight forward to navigate and display and query IMHO.

@pombredanne re

This is only a theoretical problem. Just do not do it :)

I disagree, there are instances where the sources contradict themselves, in such cases IMHO we should bring it to their notice, community curation :) . I have posted in chat about how archlinux advisory has 2 contradicting entries

Why so?
I see it this way, for a given vulnerability
ref1(NVD CVE) says the score type cvss2 is 5.0
ref1(NVD CVE) says the score type cvss3 is 6.0
ref2(RedHat CVE) says the score type cvss2 is 7.0
which becomes really straight forward to navigate and display and query IMHO.

I have a FK of 'source' on score already, so it would mean the same.